fix(propagator): touch folder paths if permissions changed

Extra safeguard to ensure that the usage of
`FileSystem::setFolderPermissions` won't start a new sync run if
FolderWatcher/inotify detects a change in the file metadata...

Signed-off-by: Jyrki Gadinger <nilsding@nilsding.org>

diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp
index 5751911e18643e75ec0231d248765a288e56a689..bd597dee8df059582222d391fa9e98d0bf8acd50 100644 (file)
--- a/src/libsync/owncloudpropagator.cpp
+++ b/src/libsync/owncloudpropagator.cpp
@@ -1468,11 +1468,14 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status)
                 !_item->_remotePerm.hasPermission(RemotePermissions::CanAddFile) &&
                 !_item->_remotePerm.hasPermission(RemotePermissions::CanAddSubDirectories)) {
                 try {
-                    if (FileSystem::fileExists(propagator()->fullLocalPath(_item->_file))) {
-                        FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_file), FileSystem::FolderPermissions::ReadOnly);
+                    if (const auto fileName = propagator()->fullLocalPath(_item->_file); FileSystem::fileExists(fileName)) {
+                        FileSystem::setFolderPermissions(fileName, FileSystem::FolderPermissions::ReadOnly);
+                        Q_EMIT propagator()->touchedFile(fileName);
                     }
                     if (!_item->_renameTarget.isEmpty() && FileSystem::fileExists(propagator()->fullLocalPath(_item->_renameTarget))) {
-                        FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_renameTarget), FileSystem::FolderPermissions::ReadOnly);
+                        const auto fileName = propagator()->fullLocalPath(_item->_renameTarget);
+                        FileSystem::setFolderPermissions(fileName, FileSystem::FolderPermissions::ReadOnly);
+                        Q_EMIT propagator()->touchedFile(fileName);
                     }
                 }
                 catch (const std::filesystem::filesystem_error &e)
@@ -1495,10 +1498,11 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status)
                 }
             } else {
                 try {
-                    const auto permissionsChangeHelper = [] (const auto fileName)
+                    const auto permissionsChangeHelper = [this] (const auto fileName)
                     {
                         qCDebug(lcDirectory) << fileName << "permissions changed: old permissions" << static_cast<int>(std::filesystem::status(fileName.toStdWString()).permissions());
                         FileSystem::setFolderPermissions(fileName, FileSystem::FolderPermissions::ReadWrite);
+                        Q_EMIT propagator()->touchedFile(fileName);
                         qCDebug(lcDirectory) << fileName << "applied new permissions" << static_cast<int>(std::filesystem::status(fileName.toStdWString()).permissions());
                     };
 

diff --git a/test/syncenginetestutils.h b/test/syncenginetestutils.h
index 44ed5edda7e3df2eb8e8a5384576e1c697a06f0e..2aaa4f5657448ed2d2ed751a20c131a41863d53f 100644 (file)
--- a/test/syncenginetestutils.h
+++ b/test/syncenginetestutils.h
@@ -575,7 +575,7 @@ public:
     [[nodiscard]] OCC::SyncJournalDb &syncJournal() const { return *_journalDb; }
     [[nodiscard]] FakeQNAM* networkAccessManager() const { return _fakeQnam; }
 
-    FileModifier &localModifier() { return _localModifier; }
+    DiskFileModifier &localModifier() { return _localModifier; }
     FileInfo &remoteModifier() { return _fakeQnam->currentRemoteState(); }
     FileInfo currentLocalState();
 

diff --git a/test/testallfilesdeleted.cpp b/test/testallfilesdeleted.cpp
index 14ca3a647b08ced9480e2a587b976b4c23174e7c..3fbf96f73bd9c76e10441611a7cd532d3e03b988 100644 (file)
--- a/test/testallfilesdeleted.cpp
+++ b/test/testallfilesdeleted.cpp
@@ -76,7 +76,7 @@ private slots:
                 fakeFolder.syncEngine().journal()->clearFileTable(); // That's what Folder is doing
             });
 
-        auto &modifier = deleteOnRemote ? fakeFolder.remoteModifier() : fakeFolder.localModifier();
+        auto &modifier = deleteOnRemote ? fakeFolder.remoteModifier() : static_cast<FileModifier&>(fakeFolder.localModifier());
         const auto childrenKeys = fakeFolder.currentRemoteState().children.keys();
         for (const auto &key : childrenKeys) {
             modifier.remove(key);
@@ -118,7 +118,7 @@ private slots:
                 callback(false);
             });
 
-        auto &modifier = deleteOnRemote ? fakeFolder.remoteModifier() : fakeFolder.localModifier();
+        auto &modifier = deleteOnRemote ? fakeFolder.remoteModifier() : static_cast<FileModifier&>(fakeFolder.localModifier());
         const auto childrenKeys = fakeFolder.currentRemoteState().children.keys();
         for (const auto &key : childrenKeys) {
             modifier.remove(key);

diff --git a/test/testblacklist.cpp b/test/testblacklist.cpp
index f1445689c983ae1f19564f2875252eae0f8f8706..ffce7f6c198aaabb537ca6352e4fca7a5d3a1ab1 100644 (file)
--- a/test/testblacklist.cpp
+++ b/test/testblacklist.cpp
@@ -46,7 +46,7 @@ private slots:
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
         ItemCompletedSpy completeSpy(fakeFolder);
 
-        auto &modifier = remote ? fakeFolder.remoteModifier() : fakeFolder.localModifier();
+        auto &modifier = remote ? fakeFolder.remoteModifier() : static_cast<FileModifier&>(fakeFolder.localModifier());
 
         int counter = 0;
         const QByteArray testFileName = QByteArrayLiteral("A/new");

diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp
index 5a53904a55ab60bf0d5aa88e134674f307f9b267..7ab6431d24e65f8ac482e1427e382404e805c1bf 100644 (file)
--- a/test/testsyncengine.cpp
+++ b/test/testsyncengine.cpp
@@ -2345,6 +2345,78 @@ private slots:
         QCOMPARE(completeSpy.findItem(fileWithoutSpaces6)->_status, SyncFileItem::Status::Success);
         QCOMPARE(completeSpy.findItem(extraFileNameWithoutSpaces)->_status, SyncFileItem::Status::Success);
     }
+
+    void testTouchedFilesWhenChangingFolderPermissionsDuringSync()
+    {
+        FakeFolder fakeFolder{FileInfo{}};
+        fakeFolder.localModifier().mkdir("directory");
+        fakeFolder.localModifier().mkdir("directory/subdir");
+        fakeFolder.remoteModifier().mkdir("directory");
+        fakeFolder.remoteModifier().mkdir("directory/subdir");
+
+        // perform an initial sync to ensure local and remote have the same state
+        QVERIFY(fakeFolder.syncOnce());
+
+        QStringList touchedFiles;
+
+        // syncEngine->_propagator is only set during a sync, which doesn't work with QSignalSpy :(
+        connect(&fakeFolder.syncEngine(), &SyncEngine::started, this, [&]() {
+            // at this point we have a propagator to connect signals to
+            connect(fakeFolder.syncEngine().getPropagator().get(), &OwncloudPropagator::touchedFile, this, [&touchedFiles](const QString& fileName) {
+                touchedFiles.append(fileName);
+            });
+        });
+
+        const auto syncAndExpectNoTouchedFiles = [&]() {
+            touchedFiles.clear();
+            QVERIFY(fakeFolder.syncOnce());
+            QCOMPARE(touchedFiles.size(), 0);
+        };
+
+        // when nothing changed expect no files to be touched
+        syncAndExpectNoTouchedFiles();
+
+        // when the remote etag of a subsubdir changes expect the parent+subdirs to be touched
+        fakeFolder.remoteModifier().findInvalidatingEtags("directory/subdir");
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(touchedFiles.size(), 2);
+        QVERIFY(touchedFiles.contains(fakeFolder.localModifier().find("directory/subdir").fileName()));
+        QVERIFY(touchedFiles.contains(fakeFolder.localModifier().find("directory").fileName()));
+
+        // nothing changed again, expect no files to be touched
+        syncAndExpectNoTouchedFiles();
+
+        // when subdir folder permissions change, expect the parent to be touched
+        touchedFiles.clear();
+        fakeFolder.remoteModifier().find("directory")->permissions = RemotePermissions::fromServerString("S");
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(touchedFiles.size(), 1);
+        QVERIFY(touchedFiles.contains(fakeFolder.localModifier().find("directory").fileName()));
+
+        // another sync without changes, expect no files to be touched
+        syncAndExpectNoTouchedFiles();
+
+        // remote etag of the subdir changed, expect the parent to be touched
+        touchedFiles.clear();
+        fakeFolder.remoteModifier().findInvalidatingEtags("directory");
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(touchedFiles.size(), 1);
+        QVERIFY(touchedFiles.contains(fakeFolder.localModifier().find("directory").fileName()));
+
+        // same as usual, expect no files to be touched
+        syncAndExpectNoTouchedFiles();
+
+        // remote rename of the subdir folder, expect the new name to be touched
+        touchedFiles.clear();
+        fakeFolder.remoteModifier().rename("directory", "renamedDirectory");
+        QVERIFY(fakeFolder.syncOnce());
+        qDebug() << touchedFiles;
+        QCOMPARE_GT(touchedFiles.size(), 1);
+        QVERIFY(touchedFiles.contains(fakeFolder.localModifier().find("renamedDirectory").fileName()));
+
+        // last sync without changes, expect no files to be touched
+        syncAndExpectNoTouchedFiles();
+    }
 };
 
 QTEST_GUILESS_MAIN(TestSyncEngine)

diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp
index 04d9456e053cfe4f45148ae60aea5cbe69186eb1..cb73f243f2f58338614bbbb45901e11f88c218e5 100644 (file)
--- a/test/testsyncmove.cpp
+++ b/test/testsyncmove.cpp
@@ -812,7 +812,7 @@ private slots:
     {
         QFETCH(bool, local);
         FakeFolder fakeFolder { FileInfo::A12_B12_C12_S12() };
-        auto &modifier = local ? fakeFolder.localModifier() : fakeFolder.remoteModifier();
+        auto &modifier = local ? static_cast<FileModifier&>(fakeFolder.localModifier()) : fakeFolder.remoteModifier();
 
         modifier.mkdir("FolA");
         modifier.mkdir("FolA/FolB");